Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116560: Add PyLong_GetSign() public function #116561

Merged
merged 18 commits into from
Jun 3, 2024

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Mar 10, 2024

PyLong_Sign() now is a wrapper to _PyLong_Sign(): argument checking added, also documentation and tests


Notes: Some PyLong_As* functions fallback to __index__() call, some not. C.f. PyLong_AsLong() vs PyLong_AsSsize_t(). Not sure if this is not a bug, but second was chosen.


📚 Documentation preview 📚: https://cpython-previews--116561.org.readthedocs.build/

* rename _PyLong_Sign() to PyLong_Sign()
* add argument checking, documentation and tests
* keep _PyLong_Sign() as an alias to PyLong_Sign()
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Include/cpython/longobject.h Outdated Show resolved Hide resolved
Lib/test/test_capi/test_long.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

I created capi-workgroup/decisions#19 for this API.

@vstinner
Copy link
Member

vstinner commented Jun 1, 2024

I created capi-workgroup/decisions#19 for this API.

The SC decided to adopt this API: capi-workgroup/decisions#19 (comment)

@skirpichev: Would you mind to update your PR for this API?

@skirpichev skirpichev requested a review from vstinner June 2, 2024 02:48
@skirpichev
Copy link
Member Author

@vstinner, I think it's already in sync with that version. Sphinx docs use slightly different wording. Let me know if you prefer variant from capi-workgroup/decisions#19 (comment).

Objects/longobject.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Jun 2, 2024

@vstinner, I think it's already in sync with that version.

Well, the C API WG prefers the name PyLong_GetSign(). Can you please rename the function?

Objects/longobject.c Outdated Show resolved Hide resolved
@vstinner vstinner changed the title gh-116560: Add PyLong_Sign() public function gh-116560: Add PyLong_GetSign() public function Jun 2, 2024
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. The overall change LGTM, but I have some more comments.

Doc/whatsnew/3.13.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.13.rst Outdated Show resolved Hide resolved
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Include/cpython/longobject.h Outdated Show resolved Hide resolved
Objects/longobject.c Outdated Show resolved Hide resolved
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if you modify the doc as suggested :-)

Doc/c-api/long.rst Outdated Show resolved Hide resolved
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Co-authored-by: Sergey B Kirpichev <[email protected]>
@vstinner vstinner enabled auto-merge (squash) June 3, 2024 07:48
@skirpichev
Copy link
Member Author

Thanks for review and patience:)

auto-merge was automatically disabled June 3, 2024 07:55

Head branch was pushed to by a user without write access

@vstinner vstinner merged commit 61d3ab3 into python:main Jun 3, 2024
36 checks passed
@vstinner
Copy link
Member

vstinner commented Jun 3, 2024

Merged, thanks.

@vstinner
Copy link
Member

vstinner commented Jun 3, 2024

Function added to pythoncapi-compat: python/pythoncapi-compat@18d1df7

@skirpichev skirpichev deleted the PyLong_Sign-116560 branch June 3, 2024 13:36
mliezun pushed a commit to mliezun/cpython that referenced this pull request Jun 3, 2024
barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants